Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

core/services/keystore: switch to sqlutil.DataStore #12810

Merged
merged 1 commit into from
Apr 19, 2024
Merged

core/services/keystore: switch to sqlutil.DataStore #12810

merged 1 commit into from
Apr 19, 2024

Conversation

jmank88
Copy link
Contributor

@jmank88 jmank88 commented Apr 13, 2024 •
edited
Loading

@jmank88 jmank88 mentioned this pull request Apr 13, 2024
Merged
1 task
@github-actions GitHub Actions
Copy link
Contributor

I see you updated files related to core. Please run pnpm changeset in the root directory to add a changeset as well as in the text include at least one of the following tags:

#nops : For any feature that is NOP facing and needs to be in the official Release Notes for the release.
#added : For any new functionality added.
#changed : For any change to the existing functionality. 
#removed : For any functionality/config that is removed.
#updated : For any functionality that is updated.
#deprecation_notice : For any upcoming deprecation functionality.
#breaking_change : For any functionality that requires manual action for the node to boot.
#db_update : For any feature that introduces updates to database schema.
#wip : For any change that is not ready yet and external communication about it should be held off till it is feature complete.
#bugfix - For bug fixes.
#internal - For changesets that need to be excluded from the final changelog.

@jmank88 jmank88 force-pushed the BCF-2978-rm-qopts-keystore branch from 98d889f to 2919c97 Compare April 13, 2024 15:28
@jmank88 jmank88 force-pushed the BCF-2978-rm-qopts-keystore branch from 2919c97 to 3ac208c Compare April 13, 2024 17:56
@jmank88 jmank88 force-pushed the BCF-2978-rm-qopts-keystore branch from 3ac208c to 6e7cdd8 Compare April 13, 2024 17:58
@jmank88 jmank88 force-pushed the BCF-2978-rm-qopts-keystore branch from 6e7cdd8 to 23004be Compare April 14, 2024 04:33
@jmank88 jmank88 force-pushed the BCF-2978-rm-qopts-keystore branch from 23004be to 8c7290d Compare April 14, 2024 12:40
@jmank88 jmank88 force-pushed the BCF-2978-rm-qopts-keystore branch from 8c7290d to 6a01834 Compare April 15, 2024 14:42
@jmank88 jmank88 force-pushed the BCF-2978-rm-qopts-keystore branch from 6a01834 to e1c5253 Compare April 16, 2024 20:40
@jmank88 jmank88 force-pushed the BCF-2978-rm-qopts-keystore branch from e1c5253 to 2ebc5b1 Compare April 16, 2024 20:47
@jmank88 jmank88 force-pushed the BCF-2978-rm-qopts-keystore branch from 2ebc5b1 to d695fde Compare April 16, 2024 23:48
@jmank88 jmank88 force-pushed the BCF-2978-rm-qopts-keystore branch from d695fde to 804f45d Compare April 17, 2024 00:31
@jmank88
Copy link
Contributor Author

jmank88 commented Apr 17, 2024

The integration tests are failing on ensure X key.

@jmank88 jmank88 force-pushed the BCF-2978-rm-qopts-keystore branch from 804f45d to e69cd4e Compare April 17, 2024 19:53
@jmank88 jmank88 force-pushed the BCF-2978-rm-qopts-keystore branch from e69cd4e to 0ad0eb5 Compare April 17, 2024 20:09
@jmank88 jmank88 force-pushed the BCF-2978-rm-qopts-keystore branch 2 times, most recently from 551d1d7 to f2b38f3 Compare April 17, 2024 20:58
patrickhuie19
patrickhuie19 reviewed Apr 19, 2024
View reviewed changes
core/services/keystore/cosmos_test.go
ks := keyStore.Cosmos()
reset := func() {
ctx := context.Background() // Executed during cleanup
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it benefit to be testutils.Context(t)? I'm not 100% on my understanding of how go test cleanup operates, but i've had some friction with debugging tests that hang/fail in cleanup vs in the test execution.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it can't be a normal ctx since we execute it during t.Cleanup, when the context may already be done.

patrickhuie19
patrickhuie19 reviewed Apr 19, 2024
View reviewed changes
core/services/keystore/eth.go
if err := q.Get(state, sql, address, chainID.String()); err != nil {
return errors.Wrap(err, "failed to insert evm_key_state")
if err := ds.GetContext(ctx, state, sql, address, chainID.String()); err != nil {
return errors.Wrap(err, "failed to insert key_state")
}
// consider: do we really need a cache of the keystates?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think we don't 😛

patrickhuie19
patrickhuie19 reviewed Apr 19, 2024
View reviewed changes
core/services/keystore/eth_test.go
@@ -27,18 +29,20 @@ import (
func Test_EthKeyStore(t *testing.T) {
t.Parallel()

db := pgtest.NewSqlxDB(t)
cfg := configtest.NewTestGeneralConfig(t)
db := sqlutil.WrapDataSource(pgtest.NewSqlxDB(t), logger.Test(t), sqlutil.TimeoutHook(func() time.Duration {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why wrap pgtest.NewSqlxDb(t)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just mimicking production. This is where I debugged the issue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's worth a code comment because it's not obvious.

or should the test db impl be changed to return the wrapper?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, in the final PR #12456 I updated cltest at least to use a timeout wrapper. We could consider updating the test funcs to return a wrapped DataSource instead of raw sqlx most places 🤷

patrickhuie19
patrickhuie19 reviewed Apr 19, 2024
View reviewed changes
core/web/jobs_controller_test.go
@@ -477,7 +480,6 @@ targets:
func TestJobsController_Create_WebhookSpec(t *testing.T) {
app := cltest.NewApplicationEVMDisabled(t)
require.NoError(t, app.Start(testutils.Context(t)))
t.Cleanup(func() { assert.NoError(t, app.Stop()) })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice - i've seen similar cleanup needed in helpers in mercury and llo. Maybe worth creating a ticket to audit (good for a new joiner?)

@jmank88 jmank88 requested a review from patrickhuie19 April 19, 2024 18:03
@jmank88 jmank88 force-pushed the BCF-2978-rm-qopts-keystore branch from a44dc6f to 33be896 Compare April 19, 2024 18:13
@cl-sonarqube-production
Copy link

Quality Gate passed Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
92.2% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

@jmank88 jmank88 added this pull request to the merge queue Apr 19, 2024
Merged via the queue into develop with commit 1fce16e Apr 19, 2024
105 checks passed
@jmank88 jmank88 deleted the BCF-2978-rm-qopts-keystore branch April 19, 2024 20:47
This was referenced Apr 24, 2024
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samsondav samsondav samsondav approved these changes

@krehermann krehermann Awaiting requested review from krehermann

@DylanTinianov DylanTinianov Awaiting requested review from DylanTinianov

@dimriou dimriou Awaiting requested review from dimriou

@patrickhuie19 patrickhuie19 Awaiting requested review from patrickhuie19

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jmank88 @samsondav @krehermann @patrickhuie19